Skip to content

Disable Field count validation of CSV viewer #35228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Birdulon
Copy link

@Birdulon Birdulon commented Aug 7, 2025

Default behaviour rejected all rows (Records) with more or fewer columns (Fields) than the first row, preventing them from parsing at all and silently hiding them. While RFC4180 section 2.4 says each line should contain the same number of fields, enforcing this on the viewer is unhelpful.
This pull request disables that validation, allowing the viewer to render lines with fewer columns than the maximum number within the file. As it's a simple HTML table, this works without additional changes (i.e. no need to manually determine the maximum number of columns), but the default appearance of rows with fewer columns may be undesirable to some people, especially when using CSS that has td {border-right: none}.
Screenshot without cell right borders
Screenshot with cell right borders

Fixes #16559, #30358.

Unfortunately, retaining empty lines is less trivial, so the line numbers on the leftmost column will still not match the source file whenever those are present, though a future PR could address that.

Default behaviour rejected all rows (Records) with more or fewer columns (Fields) than the first row, preventing them from rendering at all and silently hiding them.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 7, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 7, 2025
@silverwind
Copy link
Member

Unit tests need to be updated/extended:

--- FAIL: TestDetermineDelimiterReadAllError (0.00s)
    csv_test.go:132: 
        	Error Trace:	/home/runner/work/gitea/gitea/modules/csv/csv_test.go:132
        	Error:      	An error is expected but got nil.
        	Test:       	TestDetermineDelimiterReadAllError
        	Messages:   	RaadAll() should throw error
    csv_test.go:133: 
        	Error Trace:	/home/runner/work/gitea/gitea/modules/csv/csv_test.go:133
        	Error:      	Target error should be in err chain:
        	            	expected: "wrong number of fields"
        	            	in chain: 
        	Test:       	TestDetermineDelimiterReadAllError
    csv_test.go:134: 
        	Error Trace:	/home/runner/work/gitea/gitea/modules/csv/csv_test.go:134
        	Error:      	Should be empty, but was [[col1 col2] [a;b] [c@e] [f	g] [h|i] [jkl]]
        	Test:       	TestDetermineDelimiterReadAllError
        	Messages:   	rows should be empty

@Birdulon
Copy link
Author

Birdulon commented Aug 7, 2025

I see that test case (TestDetermineDelimiterReadAllError) is roughly the same input as TestGuessDelimiter case 16. If there's nothing else it's testing for, perhaps it should be changed to a third case for TestCreateReaderAndDetermineDelimiter?

@lunny lunny added this to the 1.25.0 milestone Aug 8, 2025
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 8, 2025
@lunny
Copy link
Member

lunny commented Aug 8, 2025

What will happen to CSV file comparisons after this change?

@Birdulon
Copy link
Author

Birdulon commented Aug 9, 2025

What will happen to CSV file comparisons after this change?

The missing columns seem to already be empty cells, perhaps because the comparison code already needed to handle changing number of columns.
image
image

Personally I find the side-by-side rendered diff to be too confusing to read and am thinking about doing row-by-row comparisons.

@silverwind
Copy link
Member

silverwind commented Aug 10, 2025

Yeah, row-based diff would be similar to git diff and likely more readable. Probably better to split that out into another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSV rendering for TSV files does not show rows with empty fields
4 participants